Skip to content

Conversation

@mtfishman
Copy link
Collaborator

This adds support for composing truncation strategies, which is then used to enable mixing truncating with rtol, atol, and maxrank.

@codecov
Copy link

codecov bot commented Apr 9, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 10 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/implementations/truncation.jl 50.00% 10 Missing ⚠️
Files with missing lines Coverage Δ
ext/MatrixAlgebraKitChainRulesCoreExt.jl 90.81% <ø> (ø)
src/algorithms.jl 80.70% <ø> (ø)
src/implementations/orthnull.jl 97.70% <ø> (ø)
src/interface/orthnull.jl 66.66% <ø> (ø)
src/pullbacks/polar.jl 100.00% <ø> (ø)
src/implementations/truncation.jl 80.88% <50.00%> (-1.81%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@lkdvos lkdvos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Something I was wondering: do we want to extend this to an arbitrary number of truncation algorithms? Ie, something like

struct TruncationComposition{T<:Tuple{Vararg{TruncationStrategy}}}
    components::T
end
TruncationComposition(component::TruncationStrategy, components::TruncationStrategy) = TruncationComposition((component, components...))

It's not strictly necessary, since you could do (trunc1 & trunc2) & trunc3 as well, and I don't think the performance would matter all that much, just a design question I guess.

As a sidenote, it seems like the JuliaFormatter v2 changes are still not entirely without change. I'm fine with switching to the new version, but let's just make that a separate commit and rebase this on that?

@mtfishman
Copy link
Collaborator Author

Something I was wondering: do we want to extend this to an arbitrary number of truncation algorithms? Ie, something like

struct TruncationComposition{T<:Tuple{Vararg{TruncationStrategy}}}
    components::T
end
TruncationComposition(component::TruncationStrategy, components::TruncationStrategy) = TruncationComposition((component, components...))

It's not strictly necessary, since you could do (trunc1 & trunc2) & trunc3 as well, and I don't think the performance would matter all that much, just a design question I guess.

Mostly I did it the current way since as you say it is just as general but the implementation is a bit simpler. I can switch over to flattening, I guess the truncation strategies should commute with each other so order of operations doesn't matter and flattening is fine.

As a sidenote, it seems like the JuliaFormatter v2 changes are still not entirely without change. I'm fine with switching to the new version, but let's just make that a separate commit and rebase this on that?

Sure, will do.

mtfishman and others added 5 commits April 9, 2025 11:39
@mtfishman
Copy link
Collaborator Author

Ok clearly I don't know how to use git, I don't know why the diff is showing all of the changes from previous PRs. But, I updated TruncationComposition based on your suggestion.

@mtfishman
Copy link
Collaborator Author

Maybe I'll just start a new PR from scratch...

@mtfishman mtfishman mentioned this pull request Apr 9, 2025
@mtfishman
Copy link
Collaborator Author

Superseded by #18.

@mtfishman mtfishman closed this Apr 9, 2025
@mtfishman mtfishman deleted the generalize_trunc branch April 9, 2025 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants